optimize blockchain subsystem scanning with threaded loading and parallel tx fetch#186
optimize blockchain subsystem scanning with threaded loading and parallel tx fetch#186abdevil05 wants to merge 2 commits into
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR extends ChangesThreaded Beldex Subsystem Block Loading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cryptonote_core/blockchain.cpp`:
- Around line 302-694: The
`Blockchain::load_missing_blocks_into_beldex_subsystems` function is doing too
many jobs at once, which makes it hard to reason about and test. Extract the
chunk-fetching logic in `get_block_data`, the threaded preloading/queue
coordination around `load_context`, and the subsystem-application loop that
updates `m_master_node_list` and `m_bns_db` into smaller helpers. Keep the main
function focused on orchestration, with clear helper boundaries for loading,
processing, and metrics/persistence.
- Around line 647-650: Gate the BNS replay in
`Blockchain::handle_block_to_main_chain` the same way MNL is gated: before
calling `m_bns_db.add_block(blk, txs)`, add a `bns_height` check so BNS
processing only runs for blocks at or above the configured BNS activation
height. Keep the existing `m_bns_db.db` guard, but ensure the block replay path
mirrors the nearby MNL height condition to avoid reprocessing historical BNS
data below `bns_height`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 40b31ee3-bf3b-40fd-b8fc-37a5b34326f6
📒 Files selected for processing (6)
src/blockchain_db/blockchain_db.hsrc/blockchain_db/lmdb/db_lmdb.cppsrc/blockchain_db/lmdb/db_lmdb.hsrc/cryptonote_core/beldex_name_system.cppsrc/cryptonote_core/blockchain.cppsrc/cryptonote_core/blockchain.h
| // --- types used for threaded loader --- | ||
| struct block_data | ||
| { | ||
| uint64_t height = 0; | ||
| std::vector<cryptonote::block> blocks; | ||
| std::vector<std::vector<cryptonote::transaction>> txs; | ||
| size_t size = 0; | ||
| bool failed = false; | ||
| }; | ||
| struct block_load_context | ||
| { | ||
| static constexpr uint64_t CHUNK_SIZE = 50; // tuneable chunk size | ||
| static constexpr size_t MAX_QUEUE_SIZE = 5; // tuneable queue depth | ||
| std::mutex block_mut; | ||
| std::condition_variable block_cv; | ||
| std::queue<block_data> next_blocks; | ||
| std::thread thread; | ||
| bool finished = false; | ||
| bool failed = false; | ||
| uint64_t height = 0; | ||
| }; | ||
| //------------------------------------------------------------------ | ||
| bool Blockchain::load_missing_blocks_into_beldex_subsystems() | ||
| bool Blockchain::load_missing_blocks_into_beldex_subsystems(const std::atomic<bool> *abort, bool use_threaded_load) | ||
| { | ||
| uint64_t const mnl_height = std::max(hard_fork_begins(m_nettype, hf::hf9_master_nodes).value_or(0), m_master_node_list.height() + 1); | ||
| uint64_t const bns_height = std::max(hard_fork_begins(m_nettype, hf::hf18_bns).value_or(0), m_bns_db.height() + 1); | ||
| uint64_t const end_height = m_db->height(); | ||
| uint64_t const start_height = std::min(end_height, std::min(bns_height, mnl_height)); | ||
|
|
||
|
|
||
| // Heights for subsystems (Beldex) | ||
| uint64_t const mnl_height = std::max(hard_fork_begins(m_nettype, hf::hf9_master_nodes).value_or(0), m_master_node_list.height() + 1); | ||
| uint64_t const bns_height = std::max(hard_fork_begins(m_nettype, hf::hf18_bns).value_or(0), m_bns_db.height() + 1); | ||
|
|
||
| const uint64_t end_height = m_db->height(); | ||
| const uint64_t start_height = std::min(end_height, std::min(bns_height, mnl_height)); | ||
| int64_t const total_blocks = static_cast<int64_t>(end_height) - static_cast<int64_t>(start_height); | ||
|
|
||
| if (total_blocks <= 0) return true; | ||
| if (total_blocks > 1) | ||
| MGINFO("Loading blocks into beldex subsystems, scanning blockchain from height: " << start_height << " to: " << end_height << " (mnl: " << mnl_height << ", bns: " << bns_height << ")"); | ||
|
|
||
| using clock = std::chrono::steady_clock; | ||
| using dseconds = std::chrono::duration<double>; | ||
| int64_t constexpr BLOCK_COUNT = 1000; | ||
| auto work_start = clock::now(); | ||
| auto scan_start = work_start; | ||
| dseconds bns_duration{}, mnl_duration{}, bns_iteration_duration{}, mnl_iteration_duration{}; | ||
| MGINFO("Loading blocks into beldex subsystems, scanning blockchain from height: " | ||
| << start_height << " to: " << end_height << " (mnl: " << mnl_height | ||
| << ", bns: " << bns_height << ")"); | ||
|
|
||
| // Initialize load context | ||
| block_load_context load_context = {}; | ||
| load_context.height = start_height; | ||
|
|
||
| for (int64_t block_count = total_blocks, | ||
| index = 0; | ||
| block_count > 0; | ||
| block_count -= BLOCK_COUNT, index++) | ||
| // Helper: fetch chunk of blocks + parallel fetch of txs | ||
| auto get_block_data = [&](uint64_t height, uint64_t end_height) -> block_data | ||
| { | ||
| auto duration = dseconds{clock::now() - work_start}; | ||
| if (duration >= 10s) | ||
| block_data next_chunk{}; | ||
| next_chunk.height = height; | ||
|
|
||
| // 1) Load blocks | ||
| { | ||
| m_master_node_list.store(); | ||
| MGINFO(fmt::format("... scanning height {} ({:.3f}s) (mnl: {:.3f}s, bns: {:.3f}s)", | ||
| start_height + (index * BLOCK_COUNT), | ||
| duration.count(), | ||
| mnl_iteration_duration.count(), | ||
| bns_iteration_duration.count())); | ||
| #ifdef ENABLE_SYSTEMD | ||
| // Tell systemd that we're doing something so that it should let us continue starting up | ||
| // (giving us 120s until we have to send the next notification): | ||
| sd_notify(0, ("EXTEND_TIMEOUT_USEC=120000000\nSTATUS=Recanning blockchain; height " + std::to_string(start_height + (index * BLOCK_COUNT))).c_str()); | ||
| #endif | ||
| work_start = clock::now(); | ||
| size_t blocks_size; | ||
|
|
||
| bns_duration += bns_iteration_duration; | ||
| mnl_duration += mnl_iteration_duration; | ||
| bns_iteration_duration = mnl_iteration_duration = {}; | ||
| if (!_get_blocks_only(height, block_load_context::CHUNK_SIZE, next_chunk.blocks, &blocks_size)) | ||
| { | ||
| LOG_ERROR("Unable to get checkpointed historical blocks [" << height << "-" << std::min<uint64_t>(height + block_load_context::CHUNK_SIZE - 1, end_height) | ||
| << "] for updating beldex subsystems"); | ||
| next_chunk.failed = true; | ||
| return next_chunk; | ||
| } | ||
| next_chunk.size += blocks_size; | ||
| } | ||
|
|
||
| std::vector<cryptonote::block> blocks; | ||
| uint64_t height = start_height + (index * BLOCK_COUNT); | ||
| if (!get_blocks_only(height, static_cast<uint64_t>(BLOCK_COUNT), blocks)) | ||
| if (next_chunk.blocks.empty()) | ||
| { | ||
| LOG_ERROR("Unable to get checkpointed historical blocks for updating beldex subsystems"); | ||
| return false; | ||
| return {}; | ||
| } | ||
|
|
||
| for (cryptonote::block const &blk : blocks) | ||
| // 2) Parallelise TX loading via threadpool (one wait per block - keeps memory modest) | ||
| tools::threadpool::waiter tpool_waiter; | ||
| tools::threadpool &tpool = tools::threadpool::getInstance(); | ||
|
|
||
| { | ||
| uint64_t block_height = get_block_height(blk); | ||
| std::atomic<size_t> bytes_loaded_for_block{0}; | ||
| std::atomic<uint64_t> failed_height{0}; | ||
| next_chunk.txs.resize(next_chunk.blocks.size()); | ||
|
|
||
| for (size_t blk_index = 0; blk_index < next_chunk.blocks.size(); blk_index++) | ||
| { | ||
| const auto &blk = next_chunk.blocks[blk_index]; | ||
| uint64_t blk_height = get_block_height(blk); | ||
| auto &txs = next_chunk.txs[blk_index]; | ||
| txs.resize(blk.tx_hashes.size()); | ||
|
|
||
| for (size_t tx_index = 0; tx_index < blk.tx_hashes.size(); ++tx_index) | ||
| { | ||
| const crypto::hash &tx_hash = blk.tx_hashes[tx_index]; | ||
| tpool.submit(&tpool_waiter, [this, &txs, tx_index, tx_hash, | ||
| &bytes_loaded_for_block, blk_height, &failed_height]() | ||
| { | ||
| std::vector<transaction> get_tx_result; | ||
| const std::vector<crypto::hash> single_hash{tx_hash}; | ||
| if (!_get_transactions(single_hash, get_tx_result, nullptr, nullptr)) { | ||
| if (failed_height == 0) failed_height = blk_height; | ||
| return; | ||
| } | ||
| if (get_tx_result.empty()) { | ||
| if (failed_height == 0) failed_height = blk_height; | ||
| return; | ||
| } | ||
| bytes_loaded_for_block += get_tx_result[0].blob_size; | ||
| txs[tx_index] = std::move(get_tx_result[0]); }); | ||
|
|
||
| if (failed_height) | ||
| break; | ||
| } | ||
|
|
||
| //CRITICAL FIX: Wait after EACH block, not after entire chunk | ||
| tpool_waiter.wait(&tpool); | ||
|
|
||
| if (failed_height) | ||
| { | ||
| LOG_ERROR("Unable to get all transactions for subsystem updating from block: " << failed_height); | ||
| next_chunk.blocks.clear(); | ||
| next_chunk.txs.clear(); | ||
| next_chunk.size = 0; | ||
| next_chunk.failed = true; | ||
| return next_chunk; | ||
| } | ||
|
|
||
| // Move block processing here | ||
| next_chunk.size += bytes_loaded_for_block.load(); | ||
| bytes_loaded_for_block = 0; // Reset for next block | ||
| cryptonote::get_block_hash(blk); | ||
| } | ||
| } | ||
| return next_chunk; | ||
| }; | ||
|
|
||
| // If using a threaded loader, spawn loader thread that preloads chunks | ||
| if (use_threaded_load) | ||
| { | ||
| load_context.thread = std::thread{[&] | ||
| { | ||
| // Deferred callback that gets fired if we return early (or throw) that makes sure the | ||
| // processing thread gets notified about the failure. | ||
| auto failure_propagator = beldex::defer([&] | ||
| { | ||
| std::unique_lock lock{load_context.block_mut}; | ||
| load_context.failed = true; | ||
| load_context.block_cv.notify_all(); | ||
| }); | ||
|
|
||
| for (; load_context.height < end_height; | ||
| load_context.height += block_load_context::CHUNK_SIZE) | ||
| { | ||
| { | ||
| std::unique_lock lock{load_context.block_mut}; | ||
| load_context.block_cv.wait( | ||
| lock, | ||
| [&] | ||
| { | ||
| return load_context.failed || (abort && *abort) || | ||
| load_context.next_blocks.size() < block_load_context::MAX_QUEUE_SIZE; | ||
| }); | ||
|
|
||
| if (load_context.failed || (abort && *abort)) | ||
| return; | ||
|
|
||
| assert(load_context.next_blocks.size() < block_load_context::MAX_QUEUE_SIZE); | ||
| } | ||
|
|
||
| // Load the block data (may be slow) | ||
| block_data next_chunk = get_block_data(load_context.height, end_height); | ||
|
|
||
| if (next_chunk.failed) | ||
| { | ||
| std::unique_lock lock{load_context.block_mut}; | ||
| load_context.failed = true; | ||
| load_context.block_cv.notify_all(); | ||
| return; | ||
| } | ||
|
|
||
| { | ||
| std::unique_lock lock{load_context.block_mut}; | ||
| load_context.next_blocks.push(std::move(next_chunk)); | ||
| } | ||
| load_context.block_cv.notify_all(); | ||
| } | ||
|
|
||
| std::vector<cryptonote::transaction> txs; | ||
| if (!get_transactions(blk.tx_hashes, txs)) | ||
| // Disarm the failure transmitter, then signal the processing thread that we finished | ||
| // loading everything. | ||
| failure_propagator.cancel(); | ||
|
|
||
| { | ||
| std::unique_lock lock{load_context.block_mut}; | ||
| load_context.finished = true; | ||
| } | ||
| load_context.block_cv.notify_all(); | ||
| }}; | ||
| } | ||
|
|
||
| // If we bail out of this function in any way before the very end successful `return true` (just | ||
| // before which we cancel this deferred call) then make sure we signal the loader thread and | ||
| // rejoin the thread on our way out. | ||
| auto failure_rejoiner = beldex::defer([&]{ | ||
| if (use_threaded_load) { | ||
| { | ||
| std::unique_lock<std::mutex> lock{load_context.block_mut}; | ||
| load_context.failed = true; | ||
| } | ||
| load_context.block_cv.notify_all(); | ||
| load_context.thread.join(); | ||
| } }); | ||
|
|
||
| // Timers / stats | ||
| using clock = std::chrono::steady_clock; | ||
| using dseconds = std::chrono::duration<double>; | ||
| auto work_start = clock::now(); | ||
| auto scan_start = work_start; | ||
| dseconds bns_duration{}, bns_interval_duration{}; | ||
| dseconds mnl_duration{}, mnl_interval_duration{}; | ||
| dseconds get_block_data_duration{}, get_block_data_interval_duration{}; | ||
|
|
||
| // Periodic store setup (matching Oxen's approach) | ||
| constexpr auto store_interval = std::chrono::minutes(5); | ||
| auto next_store = | ||
| work_start + std::chrono::milliseconds{ | ||
| crypto::rand<uint64_t>() % std::chrono::milliseconds{store_interval}.count()}; | ||
|
|
||
| uint64_t work_blocks = 0; | ||
| uint64_t total_bytes = 0, work_bytes = 0; | ||
|
|
||
| // Main processing loop: pull chunks and feed to subsystems | ||
| while (true) | ||
| { | ||
| auto get_block_data_start = clock::now(); | ||
| block_data chunk; | ||
|
|
||
| if (use_threaded_load) | ||
| { | ||
| { | ||
| MERROR("Unable to get transactions for block for updating BNS DB: " << cryptonote::get_block_hash(blk)); | ||
| std::unique_lock lock{load_context.block_mut}; | ||
| load_context.block_cv.wait(lock, [&] | ||
| { return load_context.failed || (abort && *abort) || load_context.finished || | ||
| !load_context.next_blocks.empty(); }); | ||
|
|
||
| if (load_context.failed || (abort && *abort)) | ||
| return false; | ||
|
|
||
| if (load_context.finished && load_context.next_blocks.empty()) | ||
| break; | ||
|
|
||
| chunk = std::move(load_context.next_blocks.front()); | ||
| load_context.next_blocks.pop(); | ||
| } | ||
| load_context.block_cv.notify_all(); // Notify the loader that we've removed a block | ||
| } | ||
| else | ||
| { | ||
| chunk = get_block_data(load_context.height, end_height); | ||
| if (chunk.failed) | ||
| return false; | ||
| if (load_context.height >= end_height || chunk.blocks.empty() || (abort && *abort)) | ||
| break; | ||
| load_context.height += block_load_context::CHUNK_SIZE; | ||
| } | ||
|
|
||
| auto now = clock::now(); | ||
| get_block_data_interval_duration += now - get_block_data_start; | ||
| dseconds interval_duration = now - work_start; | ||
|
|
||
| // Periodic store | ||
| if (now >= next_store) | ||
| { | ||
| auto store_start = clock::now(); | ||
| m_master_node_list.store(); | ||
| auto store_end = clock::now(); | ||
| auto elapsed = store_end - store_start; | ||
|
|
||
| if (elapsed >= 1s) | ||
| { | ||
| MGINFO(fmt::format("... stored MN state snapshot @ {} in {:.2f}s", | ||
| chunk.height, | ||
| std::chrono::duration<double>(elapsed).count())); | ||
| } | ||
|
|
||
| now = store_end; | ||
| next_store = now + store_interval; | ||
| } | ||
|
|
||
| bool every_10s = interval_duration >= 10s; | ||
| uint64_t height = chunk.height; | ||
|
|
||
| if (height + chunk.blocks.size() >= end_height || every_10s) | ||
| { | ||
| float blocks_per_s = static_cast<float>(work_blocks) / std::max<double>(1e-9, interval_duration.count()); | ||
| float bytes_per_s = static_cast<float>(work_bytes) / std::max<double>(1e-9, interval_duration.count()); | ||
|
|
||
| MGINFO(fmt::format("... scanning height {}/{} ({:.2f}s) (get blks: {:.2f}s; mnl: {:.2f}s; bns: {:.2f}s; {:.1f} blks/s; {}/s)", | ||
| height, end_height, interval_duration.count(), | ||
| get_block_data_interval_duration.count(), | ||
| mnl_interval_duration.count(), | ||
| bns_interval_duration.count(), | ||
| blocks_per_s, | ||
| tools::get_human_readable_bytes(bytes_per_s))); | ||
|
|
||
| #ifdef ENABLE_SYSTEMD | ||
| sd_notify(0, | ||
| ("EXTEND_TIMEOUT_USEC=120000000\nSTATUS=Rescanning blockchain; height " + | ||
| std::to_string(height)) | ||
| .c_str()); | ||
| #endif | ||
|
|
||
| mnl_duration += mnl_interval_duration; | ||
| bns_duration += bns_interval_duration; | ||
| get_block_data_duration += get_block_data_interval_duration; | ||
| total_bytes += work_bytes; | ||
|
|
||
| work_start = now; | ||
| get_block_data_interval_duration = mnl_interval_duration = bns_interval_duration = dseconds{0}; | ||
| work_blocks = work_bytes = 0; | ||
| } | ||
|
|
||
| // Feed subsystem processing | ||
| work_blocks += chunk.blocks.size(); | ||
| work_bytes += chunk.size; | ||
|
|
||
| for (size_t i = 0; i < chunk.blocks.size(); i++) | ||
| { | ||
| const auto &blk = chunk.blocks[i]; | ||
| uint64_t block_height = get_block_height(blk); | ||
| const auto &txs = chunk.txs[i]; | ||
|
|
||
| if (block_height >= mnl_height) | ||
| { | ||
| auto mnl_start = clock::now(); | ||
|
|
||
| checkpoint_t *checkpoint_ptr = nullptr; | ||
| checkpoint_t checkpoint; | ||
| if (blk.major_version >= hf::hf15_flash && get_checkpoint(block_height, checkpoint)) | ||
| checkpoint_ptr = &checkpoint; | ||
| checkpoint_ptr = &checkpoint; | ||
|
|
||
| try { | ||
| try | ||
| { | ||
| m_master_node_list.block_add(blk, txs, checkpoint_ptr); | ||
| } catch (const std::exception& e) { | ||
| MFATAL("Unable to process block {} for updating master node list: " << e.what()); | ||
| } | ||
| catch (const std::exception &e) | ||
| { | ||
| MFATAL("Unable to process block " << block_height << " for updating master node list: " << e.what()); | ||
| return false; | ||
| } | ||
| mnl_iteration_duration += clock::now() - mnl_start; | ||
| mnl_interval_duration += clock::now() - mnl_start; | ||
| } | ||
|
|
||
| if (m_bns_db.db && (block_height >= bns_height)) | ||
| if (m_bns_db.db) | ||
| { | ||
| auto bns_start = clock::now(); | ||
| if (!m_bns_db.add_block(blk, txs)) | ||
| { | ||
| MERROR("Unable to process block for updating BNS DB: " << cryptonote::get_block_hash(blk)); | ||
| return false; | ||
| } | ||
| bns_iteration_duration += clock::now() - bns_start; | ||
| bns_interval_duration += clock::now() - bns_start; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (total_blocks > 1) | ||
| { | ||
| MGINFO(fmt::format("Done recalculating beldex subsystems in {:.2f}s ({:.2f}s mnl; {:.2f}s bns)", | ||
| dseconds{clock::now() - scan_start}.count(), mnl_duration.count(), bns_duration.count())); | ||
| } | ||
| } // end main while | ||
|
|
||
| auto end = clock::now(); | ||
|
|
||
| if (total_blocks > 0) | ||
| { | ||
| if (m_master_node_list.height() != end_height - 1 || | ||
| (m_bns_db.db && m_bns_db.height() != end_height - 1)) | ||
| { | ||
| LOG_ERROR("Mismatched subsystem height after subsystem refresh: blockchain (" << end_height - 1 | ||
| << "), MNL (" << m_master_node_list.height() | ||
| << "), BNS (" << m_bns_db.height() << ")"); | ||
| return false; | ||
| } | ||
|
|
||
| total_bytes += work_bytes; | ||
| dseconds duration{end - scan_start}; | ||
| float blocks_per_s = static_cast<float>(total_blocks) / std::max<double>(1e-9, duration.count()); | ||
| float bytes_per_s = static_cast<float>(total_bytes) / std::max<double>(1e-9, duration.count()); | ||
| MGINFO(fmt::format("Done recalculating beldex subsystems in {:.2f}s (get blks: {:.2f}s; mnl: {:.2f}s; bns: {:.2f}s; {:.1f} blks/s; {}/s)", | ||
| duration.count(), | ||
| get_block_data_duration.count(), | ||
| mnl_duration.count(), | ||
| bns_duration.count(), | ||
| blocks_per_s, | ||
| tools::get_human_readable_bytes(bytes_per_s))); | ||
|
|
||
| m_master_node_list.store(); | ||
| } | ||
|
|
||
| if (use_threaded_load) | ||
| { | ||
| failure_rejoiner.cancel(); | ||
| assert(load_context.finished); | ||
| load_context.thread.join(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Consider extracting the loader and chunk processing helpers.
This function now mixes queue coordination, tx fetching, subsystem application, persistence, and metrics, making the refresh path hard to reason about and test. Extracting get_block_data, threaded loading, and subsystem application would reduce risk for future fixes.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 303-303: constructor does not initialize these fields: blocks, txs
(cppcoreguidelines-pro-type-member-init)
[warning] 324-324: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 324-324: function 'load_missing_blocks_into_beldex_subsystems' has cognitive complexity of 87 (threshold 25)
(readability-function-cognitive-complexity)
[note] 335-335: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 346-346: nesting level increased to 1
(clang)
[note] 355-355: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 365-365: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 379-379: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 412-412: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 432-432: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 434-434: nesting level increased to 2
(clang)
[note] 438-438: nesting level increased to 3
(clang)
[note] 445-445: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 458-458: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 458-458: +1
(clang)
[note] 458-458: +1
(clang)
[note] 461-461: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 467-467: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 497-497: nesting level increased to 1
(clang)
[note] 498-498: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 526-526: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 531-531: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 539-539: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 539-539: +1
(clang)
[note] 539-539: +1
(clang)
[note] 542-542: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 542-542: +1
(clang)
[note] 550-550: +1, nesting level increased to 2
(clang)
[note] 553-553: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 555-555: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 555-555: +1
(clang)
[note] 555-555: +1
(clang)
[note] 565-565: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 572-572: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 586-586: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 586-586: +1
(clang)
[note] 620-620: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 626-626: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 632-632: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 639-639: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 647-647: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 650-650: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 663-663: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 665-665: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 665-665: +1
(clang)
[note] 666-666: +1
(clang)
[note] 689-689: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 692-692: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[warning] 328-328: variable 'mnl_height' is not initialized
(cppcoreguidelines-init-variables)
[warning] 329-329: variable 'bns_height' is not initialized
(cppcoreguidelines-init-variables)
[warning] 335-335: statement should be inside braces
(readability-braces-around-statements)
[warning] 353-353: variable 'blocks_size' is not initialized
(cppcoreguidelines-init-variables)
[warning] 371-371: variable 'tpool_waiter' is not initialized
(cppcoreguidelines-init-variables)
[warning] 382-382: variable 'blk_height' is not initialized
(cppcoreguidelines-init-variables)
[warning] 458-458: implicit conversion 'const std::atomic *' -> bool
(readability-implicit-bool-conversion)
[warning] 458-458: statement should be inside braces
(readability-braces-around-statements)
[warning] 512-512: variable 'bns_duration' is not initialized
(cppcoreguidelines-init-variables)
[warning] 513-513: variable 'mnl_duration' is not initialized
(cppcoreguidelines-init-variables)
[warning] 514-514: variable 'get_block_data_duration' is not initialized
(cppcoreguidelines-init-variables)
[warning] 523-523: multiple declarations in a single statement reduces readability
(readability-isolate-declaration)
[warning] 539-539: implicit conversion 'const std::atomic *' -> bool
(readability-implicit-bool-conversion)
[warning] 539-539: statement should be inside braces
(readability-braces-around-statements)
[warning] 542-542: statement should be inside braces
(readability-braces-around-statements)
[warning] 553-553: statement should be inside braces
(readability-braces-around-statements)
[warning] 555-555: implicit conversion 'const std::atomic *' -> bool
(readability-implicit-bool-conversion)
[warning] 555-555: statement should be inside braces
(readability-braces-around-statements)
[warning] 562-562: variable 'interval_duration' is not initialized
(cppcoreguidelines-init-variables)
[warning] 583-583: variable 'every_10s' is not initialized
(cppcoreguidelines-init-variables)
[warning] 588-588: variable 'blocks_per_s' is not initialized
(cppcoreguidelines-init-variables)
[warning] 589-589: variable 'bytes_per_s' is not initialized
(cppcoreguidelines-init-variables)
[warning] 623-623: variable 'block_height' is not initialized
(cppcoreguidelines-init-variables)
[warning] 632-632: statement should be inside braces
(readability-braces-around-statements)
[warning] 647-647: implicit conversion 'sqlite3 *' -> bool
(readability-implicit-bool-conversion)
[warning] 666-666: implicit conversion 'sqlite3 *' -> bool
(readability-implicit-bool-conversion)
[warning] 675-675: variable 'duration' is not initialized
(cppcoreguidelines-init-variables)
[warning] 676-676: variable 'blocks_per_s' is not initialized
(cppcoreguidelines-init-variables)
[warning] 677-677: variable 'bytes_per_s' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.21.0)
[error] 335-335: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cryptonote_core/blockchain.cpp` around lines 302 - 694, The
`Blockchain::load_missing_blocks_into_beldex_subsystems` function is doing too
many jobs at once, which makes it hard to reason about and test. Extract the
chunk-fetching logic in `get_block_data`, the threaded preloading/queue
coordination around `load_context`, and the subsystem-application loop that
updates `m_master_node_list` and `m_bns_db` into smaller helpers. Keep the main
function focused on orchestration, with clear helper boundaries for loading,
processing, and metrics/persistence.
Source: Linters/SAST tools
| if (m_bns_db.db) | ||
| { | ||
| auto bns_start = clock::now(); | ||
| if (!m_bns_db.add_block(blk, txs)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Gate BNS replay by bns_height.
start_height can be driven by the lower MNL height, but BNS is processed for every chunk block. This replays BNS below bns_height, unlike the MNL guard above.
Proposed fix
- if (m_bns_db.db)
+ if (m_bns_db.db && block_height >= bns_height)
{
auto bns_start = clock::now();
if (!m_bns_db.add_block(blk, txs))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (m_bns_db.db) | |
| { | |
| auto bns_start = clock::now(); | |
| if (!m_bns_db.add_block(blk, txs)) | |
| if (m_bns_db.db && block_height >= bns_height) | |
| { | |
| auto bns_start = clock::now(); | |
| if (!m_bns_db.add_block(blk, txs)) |
🧰 Tools
🪛 Clang (14.0.6)
[note] 647-647: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 650-650: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[warning] 647-647: implicit conversion 'sqlite3 *' -> bool
(readability-implicit-bool-conversion)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cryptonote_core/blockchain.cpp` around lines 647 - 650, Gate the BNS
replay in `Blockchain::handle_block_to_main_chain` the same way MNL is gated:
before calling `m_bns_db.add_block(blk, txs)`, add a `bns_height` check so BNS
processing only runs for blocks at or above the configured BNS activation
height. Keep the existing `m_bns_db.db` guard, but ensure the block replay path
mirrors the nearby MNL height condition to avoid reprocessing historical BNS
data below `bns_height`.
2 participants
Purpose
Core Changes
Threaded Block Loading (Pipeline Execution)
Parallel Transaction Fetching
Improved Logging and Metrics
Periodic State Persistence
Performance Impact
Testing Performed
Summary